Open Bug 1422418 Opened 7 years ago Updated 3 years ago

clang-format doesn't treat template functions return type the same as for functions

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: glandium, Unassigned)

References

Details

Start with: void foo(void) { } template<typename T> void bar(T) { } clang-format reformats it as: void foo(void) { } template<typename T> void bar(T) { } This might look trivial, but for more contrived examples, where this goes the line length limit, it changes things significantly: verylongtype<with, template, arguments, all, the, way> long_function_name(void) { } template<typename Typename> verylongtype<with, template, arguments, all, the, way> long_function_name(Typename) { } becomes: verylongtype<with, template, arguments, all, the, way> long_function_name(void) { } template<typename Typename> verylongtype<with, template, arguments, all, the, way> long_function_name( Typename) { }
Good catch Mike, the issue is that when we pass to the function declaration types from the template argument list clang-format doesn't know to distinguish token long_function_name as a 'TT_FunctionDeclarationName' and it considers it to be 'TT_StartOfName' which is obviously wrong, since our logic to break before return type is like: >> if (!Current->MustBreakBefore && InFunctionDecl && >> Current->is(TT_FunctionDeclarationName)) >> Current->MustBreakBefore = mustBreakForReturnType(Line); I'm gonna look to see if we can change the way how we generate token: 'TT_FunctionDeclarationName'
One more thing that i can note here this is almost an honest mistake since in all our cases only type is present in the function declaration parameters, if we were to have something like: >>template<typename Typename> >>verylongtype<with, template, arguments, all, the, way> long_function_name(Typename var) >>{ >>} This would be formatted to: >>template<typename Typename> >>verylongtype<with, template, arguments, all, the, way> >>long_function_name(Typename a) >>{ >>} This is due to the fact that clang-format uses a token stream for different token types recognition, and for our case there are several possibilities: 1. Only type is present so type must be part of the list of implicit types. 2. Type is also qualified so the token is deduced to be part of an argument list. 3. Token is not recognised so the starting token cannot be deduced as 'TT_FunctionDeclarationName' Also we use a top level breaking point for the return type so I don't see the usage of constructions like this in top level places but only in nested levels: >>template<typename Typename> >>verylongtype<with, template, arguments, all, the, way> long_function_name(Typename) { >>} If I missed a place were we could use them please let me know.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2) > One more thing that i can note here this is almost an honest mistake since > in all our cases only type is present in the function declaration > parameters, if we were to have something like: > > >>template<typename Typename> > >>verylongtype<with, template, arguments, all, the, way> long_function_name(Typename var) > >>{ > >>} > > This would be formatted to: > > >>template<typename Typename> > >>verylongtype<with, template, arguments, all, the, way> > >>long_function_name(Typename a) > >>{ > >>} I was about to say that's not at all how that ends up formatted here, but in fact, I found another bug: .cc files are not treated the same as .cpp files. With the .cc extension, your example is not formatted like that, while it is with a .cpp extension.
Actually, .cc files are left untouched.
> Actually, .cc files are left untouched. Yeah, we don't touch those extensions as, afaik, we don't have any in our source code (only in thirdparty) https://dxr.mozilla.org/mozilla-central/source/tools/mach_commands.py#268
Product: Core → Firefox Build System
Blocks: clang-format
Component: Source Code Analysis → Lint and Formatting
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.